Skip to content

gh-151907: Avoid creating temporary objects in some comprehensions#151908

Merged
ZeroIntensity merged 14 commits into
python:mainfrom
ZeroIntensity:comprehension-avoid-creation
Jul 14, 2026
Merged

gh-151907: Avoid creating temporary objects in some comprehensions#151908
ZeroIntensity merged 14 commits into
python:mainfrom
ZeroIntensity:comprehension-avoid-creation

Conversation

@ZeroIntensity

@ZeroIntensity ZeroIntensity commented Jun 22, 2026

Copy link
Copy Markdown
Member

The following code:

[i for i in range(42)]

is essentially turned into:

for i in range(42):
    pass

Comment thread Python/codegen.c
@iritkatriel

Copy link
Copy Markdown
Member

I think this might change semantics for the dict case - if a key is not hashable.

@ZeroIntensity

Copy link
Copy Markdown
Member Author

Yeah, good catch. This breaks set too:

{x for x in [[]]}  # No error!

I'll just remove set and dict for now, but it might still be possible to avoid object creation by calling PyObject_Hash. Not worth doing that here though.

This was a breaking change because hash() was no longer called on the
elements.
@iritkatriel

Copy link
Copy Markdown
Member

Yeah, good catch. This breaks set too:

{x for x in [[]]}  # No error!

I'll just remove set and dict for now, but it might still be possible to avoid object creation by calling PyObject_Hash. Not worth doing that here though.

Maybe also add tests for these cases.

Comment thread Lib/test/test_compiler_codegen.py Outdated
@iritkatriel

Copy link
Copy Markdown
Member

The async case is not handled correctly here. This segfaults:

import asyncio
async def foo(XS):
    [x async for x in XS]

async def agen():
     yield 1
     yield 2
asyncio.run(foo(agen()))
Assertion failed: (PyList_Check(self)), function _PyList_AppendTakeRef, file pycore_list.h, line 40.
zsh: abort      ./python.exe

@ZeroIntensity

Copy link
Copy Markdown
Member Author

Fixed.

Avoids an "env-changed" error with regrtest.
Comment thread Lib/test/test_compiler_codegen.py Outdated
Comment thread Python/codegen.c Outdated
if (value->kind == ListComp_kind) {
/* Optimization: Don't bother creating structures if they won't be
* used. */
return codegen_listcomp_impl(c, value, /*avoid_creation=*/true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nicer if we were not calling codegen_listcomp_impl here but rather setting a boolean on the compiler_unit struct to track whether we are in the topmost expression or not. What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not opposed to the idea, but it feels slightly weird in cases where the thing being compiled isn't an expression. A compiler->has_no_target flag in a FunctionDef, for example, has no real meaning (because compiler->has_no_target == 0 would imply "has target", but that doesn't make sense for a statement). I do think it'll make future optimizations like this easier, though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a flag like is_subexpression.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would I implement that? I can't do like METADATA(c)->u_is_subexpression = 1 in codegen_stmt_expr because that would also affect listcomps inside the one we're optimizing (like [x for x in [y for y in z]]). I could add a stack or something like that, but that would significantly increase the complexity of this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like this:

static int
codegen_visit_expr(compiler *c, expr_ty e)
{
    if (Py_EnterRecursiveCall(" during compilation")) {
        return ERROR;
    }
    return codegen_visit_expr_impl(c, e, false);
}

static int
codegen_visit_unused_expr(compiler *c, expr_ty e)
{
    return codegen_visit_expr_impl(c, e, true);
}

static int
codegen_visit_expr_impl(compiler *c, expr_ty e, bool result_is_unused)
{
#define VISIT_UNUSED(C, TYPE, V) \
    RETURN_IF_ERROR(codegen_visit_unused_ ## TYPE((C), (V)))

and then in codegen_stmt_expr use VISIT_UNUSED instead of VISIT?

Then codegen_stmt_expr doesn't need to do anything special for ListComp_kind.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea. I just pushed it, let me know what you think.

ZeroIntensity and others added 2 commits June 30, 2026 13:27
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Comment thread Python/codegen.c
Comment thread Python/codegen.c
Comment thread Python/codegen.c Outdated
case COMP_LISTCOMP:
if (avoid_creation) {
if (elt->kind == Starred_kind) {
RETURN_IF_ERROR(codegen_starred_comprehension(c, elt_loc, elt->v.Starred.value, /*discard=*/true));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. Why don't we just do VISIT(c, expr, elt->v.Starred.value); like below for the !avoid_creation case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elt->v.Starred.value only creates an iterator object, but we need to exhaust the whole thing in order to get all the side effects. It wasn't necessary before because LIST_EXTEND does the unpacking for us.

Comment thread Python/codegen.c Outdated
}

static int
codegen_starred_comprehension(compiler *c, location loc, expr_ty value, bool discard)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name doesn't seem correct. This function is not actually about comprehensions, right?

Comment thread Python/codegen.c Outdated
Comment on lines +4572 to +4577
if (discard) {
ADDOP(c, loc, POP_TOP);
} else {
ADDOP_YIELD(c, loc);
ADDOP(c, loc, POP_TOP);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just do this (and change the arg name/meaning)?

Suggested change
if (discard) {
ADDOP(c, loc, POP_TOP);
} else {
ADDOP_YIELD(c, loc);
ADDOP(c, loc, POP_TOP);
}
if (yield) {
ADDOP_YIELD(c, loc);
}
ADDOP(c, loc, POP_TOP);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@iritkatriel

Copy link
Copy Markdown
Member

maybe run ref leaks tests just to make sure.

@ZeroIntensity ZeroIntensity added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 14, 2026
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit e86a7be 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F151908%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 14, 2026
@ZeroIntensity ZeroIntensity merged commit a785898 into python:main Jul 14, 2026
73 of 74 checks passed
@ZeroIntensity ZeroIntensity deleted the comprehension-avoid-creation branch July 14, 2026 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants